Every presentation a command declares is required - #171
Conversation
`Presentations` made `stdout`, `json` and `next` optional, so a command that omitted `json` got its `--json` contract by accident: the engine published the handler's internal `data` object as the envelope's `result`. The surface machine consumers depend on was set by omission rather than by choice. All four are now required, and every command states what it publishes on each surface. Behaviour is preserved exactly: where a command had no `json` it now returns what the handler passes as `data`, because that is what the envelope carried; where it had no `stdout` or `next` it returns an empty array, because that is what it wrote. The engine's settlement fallback is gone — `result` is the json presentation, not a substitution. One fallback stays, and it is commented where it lives: `materializePresentation` still calls `next` optionally, because @prisma/orm-toolchain@8.0.0-rc.1-dev.40 is compiled against an earlier engine and declares no `next` for `migration list`. Requiring it makes that command exit 2. `stdout` and `json` are required at runtime, which both published families already satisfy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Both package test scripts now begin with pnpm run build, so turbo runs the engine's build while the CLI's vitest imports the dist it is rewriting. The failure is "Cannot find package '@prisma/cli-engine/testing'" at file level, and the count varies: 13, 34 and 43 across three runs of one branch. Serial turbo and a direct vitest run both pass all 60. The existing entry said the CLI script was a bare vitest run and that turbo was therefore honest. The build step has since been added to both scripts, which is what created the race. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
commit: |
Summary by CodeRabbit
WalkthroughThe presentation contract now requires 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.drive/projects/prisma-cli-v8/deferred.md:
- Line 168: Consolidate the duplicate shared-dist build-race documentation by
keeping one canonical entry and removing or merging the redundant entry around
the documented race. Align the canonical entry’s reproduction command, observed
failure message, and remediation with the corresponding details in the other
entry, preserving the recommendation to eliminate concurrent builds of the
shared dist directory.
In `@packages/cli-engine/src/execution/command-context.ts`:
- Around line 58-59: Complete the comment describing the next field’s
compatibility behavior: state that next is the only field that may be absent at
runtime while the other fields remain required, and preserve the surrounding
Presentations requirement.
- Around line 77-83: Update the presentation materialization flow to pass
outcome.data into materializePresentation, and in the returned presentation
object invoke json and stdout presenters optionally to support legacy
Presentations objects. Preserve the existing outputs while defaulting missing
stdout to [] and avoiding runtime errors when json or stdout is absent.
In `@packages/cli-engine/src/execution/settlement.ts`:
- Line 74: Add or update a regression test for the settlement flow around the
JSON presentation handler, using distinct `data` and `json` fixture values and
asserting that the completed JSON envelope’s `result` matches the JSON handler’s
returned value. Ensure the test would fail if the implementation regresses to
using presented.data.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c65e6df2-870f-4a44-8ca7-a9a7777b109b
📒 Files selected for processing (69)
.drive/projects/prisma-cli-v8/deferred.mdpackages/cli-engine/src/execution/command-context.tspackages/cli-engine/src/execution/settlement.tspackages/cli-engine/src/presentation.tspackages/cli-engine/src/telemetry/commands.tspackages/cli-engine/tests/blocks.test.tspackages/cli-engine/tests/clack-isolation.test.tspackages/cli-engine/tests/clack-prompts.test.tspackages/cli-engine/tests/command-capabilities.test-d.tspackages/cli-engine/tests/command-families.test.tspackages/cli-engine/tests/config.test.tspackages/cli-engine/tests/credential-manager.test.tspackages/cli-engine/tests/engine.type-test.tspackages/cli-engine/tests/events.test.tspackages/cli-engine/tests/execution.test.tspackages/cli-engine/tests/interaction-affordances.test.tspackages/cli-engine/tests/lifetimes.test.tspackages/cli-engine/tests/management-api.test.tspackages/cli-engine/tests/package-install-matrix.test.tspackages/cli-engine/tests/package-operations.test.tspackages/cli-engine/tests/palette.test.tspackages/cli-engine/tests/prompts.test.tspackages/cli-engine/tests/redirects.test.tspackages/cli-engine/tests/run-hooks.test.tspackages/cli-engine/tests/spawn.test.tspackages/cli-engine/tests/telemetry-payload.test.tspackages/cli-engine/tests/telemetry-run.test.tspackages/cli/src/commands/agent/presentation.tspackages/cli/src/commands/auth/login.tspackages/cli/src/commands/auth/logout.tspackages/cli/src/commands/auth/whoami.tspackages/cli/src/commands/auth/workspace-logout.tspackages/cli/src/commands/auth/workspace-use.tspackages/cli/src/commands/branch/list.tspackages/cli/src/commands/bucket/create.tspackages/cli/src/commands/bucket/delete.tspackages/cli/src/commands/bucket/key-create.tspackages/cli/src/commands/bucket/key-delete.tspackages/cli/src/commands/bucket/key-list.tspackages/cli/src/commands/bucket/list.tspackages/cli/src/commands/feedback.tspackages/cli/src/commands/git/connect.tspackages/cli/src/commands/git/disconnect.tspackages/cli/src/commands/init/presentation.tspackages/cli/src/commands/postgres/backup-list.tspackages/cli/src/commands/postgres/connection-create.tspackages/cli/src/commands/postgres/connection-list.tspackages/cli/src/commands/postgres/connection-remove.tspackages/cli/src/commands/postgres/connection-rotate.tspackages/cli/src/commands/postgres/create.tspackages/cli/src/commands/postgres/list.tspackages/cli/src/commands/postgres/remove.tspackages/cli/src/commands/postgres/restore.tspackages/cli/src/commands/postgres/show.tspackages/cli/src/commands/postgres/usage.tspackages/cli/src/commands/project/env-add.tspackages/cli/src/commands/project/env-remove.tspackages/cli/src/commands/project/env-shared.tspackages/cli/src/commands/project/env-update.tspackages/cli/src/commands/project/presentation.tspackages/cli/src/commands/project/remove.tspackages/cli/src/commands/project/rename.tspackages/cli/src/commands/project/show.tspackages/cli/src/commands/project/transfer.tspackages/cli/src/commands/service/presentation.tspackages/cli/tests/auth.test.tspackages/cli/tests/golden-rendering.test.tspackages/cli/tests/mount-coverage.test.tspackages/cli/tests/whoami.test.ts
Every fixture passed the same value as `data` and as the json presentation, so none of them could tell which one the envelope published. A new fixture makes them differ, and the test fails if settlement goes back to publishing `data`. Verified by making settlement publish `data` and watching it fail, after rebuilding — the engine's own tests resolve the engine through `dist`, so a source edit alone proves nothing. Also restores the deferred entry about a single filter reporting green against a stale engine build, which the previous commit deleted by mistake. The concurrency race it was replaced with was already recorded under S6, in more detail; that entry gains the second error message and the varying failure count seen here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…answers to Spec R7 requires product-repo e2e in prisma/prisma and composer, and says nothing about the grammar this binary ships. The binary answers to 89 commands, counted by walking --help from the root through every group. 40 have an e2e test. 49 do not. 26 of the 49 come from other packages — 22 from orm-toolchain, 4 from composer — and none of them is run by this repo's e2e suite at all, though it is this binary that breaks when a mount breaks. #171 found that the hard way: requiring next at runtime made migration list exit 2, caught only by an in-process mount test. The other 23 are this repo's own, listed with what each needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
… human mode I reported that requiring `stdout` at runtime was safe because the ORM mount test passed. That test runs in json mode, and json mode never calls `stdout` — so it could not have detected the failure it was cited as ruling out. orm-toolchain's `migration list` declares `human` and `json` and neither of the other two. In human mode, the default for a user at a terminal, the strict call made it exit 2. `stdout` now uses the same optional call as `next`. `json` stays strict: every ORM command declares one, and a missing json presentation is the defect this change removes. The mount test gains a human-mode run of the same command, because the two formats call different presentation functions and a run in one proves nothing about the other. The real fix is not this shim. orm-toolchain is our code; declaring the missing presentations there and building it against this engine makes the type refuse to compile without them. Both `?.()` come out when that version is pinned here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…e engine fill them in (prisma#30004) Here is the entire change, repeated across 21 files: ```diff // src/orm/migration/list.ts function listPresentations(inputs: { … }): Presentations { return { human: (): readonly Block[] => [ { kind: 'fields', rail: true, rows: [ … ] }, { kind: 'drawing', lines: toneDrawing(inputs.tree) }, ], + stdout: () => [], + next: () => [], json: () => inputs.list, }; } ``` ## What those four things are Every CLI command hands the engine four ways of reporting what it did, and the engine uses the ones the run needs: | | what it is | who reads it | | --- | --- | --- | | `human` | the blocks a person sees, on stderr | someone at a terminal | | `stdout` | plain data lines, on stdout | a pipe, a script | | `json` | the `result` inside the `--json` envelope | a program parsing output | | `next` | suggested follow-up commands | the person, after the command finishes | Today `human` is required and the other three are optional. When a command leaves one out, the engine substitutes a default — an empty list for `stdout` and `next`. ## The decision **The engine is dropping those defaults and requiring all four.** A command will state what it publishes on each channel rather than inheriting it by omission. This PR does that stating for the ORM commands. Nothing else changes. ## Why it matters, concretely Once the engine stops substituting, it calls whatever the command declared. Our commands declare two of the four, so it calls functions that are not there: ``` $ prisma migration list # exit 2 ``` That is `stdout` in human mode — the default for anyone at a terminal — and `next` in both modes. It affects the 21 command files in this diff. Worth noting how it was found, because it says something about our coverage: **this package's own suite passes either way.** It never sees the engine version it will be mounted against. The failure only appears when you run the assembled `prisma` binary, which neither repo tests today. ## Behaviour is identical Every added `stdout` and `next` returns the same empty array the engine was already substituting. This is a no-op at runtime, on purpose. What it buys is visibility. `stdout` is the channel a script reads, and today every ORM command returns nothing on it — not as a decision, but because nobody wrote one. After this, that gap is a visible `() => []` in each file, and a command that ought to emit machine-readable lines can be given them one at a time, deliberately and reviewably. ## Timing: this does not wait on anything The three fields are optional in the engine we pin (`0.0.9`) and in the newest published engine (`8.0.0-rc.1`, on the `next` tag) alike. The version that requires them is not published yet — it is [prisma-cli#171](prisma/prisma-cli#171), still open. So this can merge and ship now, and the engine can then require the fields without breaking us. ## How I know nothing was missed I made the four fields required in the installed engine's type declarations and typechecked the package against that — the exact condition the new engine imposes. Zero errors across `src/` afterwards, and it caught one site a grep had missed: a fixture in `test/orm/define-command.test.ts`. The declarations were then restored, so nothing in this diff touches `node_modules`. `pnpm build`, `typecheck` and `lint` clean; **155 test files, 1943 tests passing.** ## Alternatives considered **Handle it in prisma-cli instead, by calling the missing channels defensively.** That shim exists today and works. Rejected as the permanent answer: it stands in for a declaration we can simply write in our own code, and it keeps the engine guessing on behalf of every ORM command we add from here. **Wait for the strict engine to publish and do this together with the version bump.** Rejected: the fields are optional in every published engine, so this ships now. Waiting only keeps the consumer shimmed for longer and couples two releases that need not be coupled. **Bump the engine pin from `0.0.9` to `8.0.0-rc.1` in the same PR.** Worth doing — an install of `@prisma/cli` currently carries two copies of the engine because this package pins `0.0.9` while prisma-cli builds `8.0.0-rc.1` — but it is a large version jump with its own surface changes, and it is not needed to make the presentations correct. Kept separate so this diff stays a reviewable no-op. **Give the commands real `stdout` output now rather than `() => []`.** Rejected for this PR. What `migration list` should print to a pipe is a genuine per-command design question and a behaviour change deserving its own review. `() => []` is exactly what ships today, which keeps this diff a no-op reviewable in one pass. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Presentationsmadestdout,jsonandnextoptional, so a command that omittedjsongot its--jsoncontract by accident: the engine published the handler's internaldataobject as the envelope'sresult. The surface machine consumers script against was set by omission rather than by choice.All four are now required in the type. Every command states what it publishes on each surface.
Behaviour is preserved exactly
Where a command had no
json, it now returns what the handler passes asdata, because that is what the envelope carried. Where it had nostdoutornext, it returns an empty array, because that is what it wrote. The point is to make each surface a stated choice, so a wrong one can then be corrected one command at a time — not to change any output here.The engine's settlement fallback is gone:
resultis the json presentation, not a substitution for a missing one, and a new test pins that by making a command'sdataandjsondiffer.Two runtime shims, and a correction
materializePresentationstill callsstdoutandnextoptionally.@prisma/orm-toolchainis built against engine0.0.9, where three of the four were optional, and its commands took that up —migration listdeclareshumanandjsonand neither of the others. Calling them unconditionally makes it exit 2:stdoutin human mode,nextin both.An earlier revision of this PR claimed
stdoutwas safe to require. That was wrong, and the way it was wrong matters: I checked it againsttests/orm-mount.test.ts, which runs in json mode — and json mode never callsstdout. The test could not have detected the failure it was cited as ruling out. Human mode, the default for anyone at a terminal, would have exited 2 across 18 ORM commands.The mount test now runs the same command in both formats, with a comment saying why: the two formats call different presentation functions, so a run in one proves nothing about the other.
jsonstays strict. Every ORM command declares one, and a missing json presentation is the defect this change exists to remove.The shims are temporary, and the real fix is ours
This is version skew in our own code, not a foreign contract.
@prisma/orm-toolchainlives inprisma/prismaand pins@prisma/cli-engine@0.0.9. Declaring the missing presentations there — 18 files lackstdout, 16 lacknext— and building it against this engine makes the type refuse to compile without them, which is where the enforcement belongs. Both?.()come out when that version is pinned here.Scope
156 call sites: 95 in the engine (93 of them test fixtures that declared only
human) and 61 in the CLI, concentrated insrc/commands/service/presentation.ts— the service group was written after this change was first attempted on a branch that never merged.Four auth commands and the shared env-file presentation needed care rather than a mechanical edit: their builders take a derived object, not the handler's
data, so returning the builder's own parameter would have changed the--jsonoutput. They now take the result explicitly.A hazard for whoever runs the tests
npm testfails at random on this repo, and it is not this change. Both packagetestscripts begin withpnpm run build, so turbo runs the engine's build while the CLI's vitest imports thedistit is rewriting.npx turbo run test --concurrency=1passes all 94 files every time. Already recorded indeferred.mdunder S6.Verified: build, lint, typecheck clean; 60 CLI test files and 34 engine test files pass serially.